-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(dynamic-group-by): scope chart refreshes to affected charts only #36955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #a7fcbbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Nitpicks 🔍
|
superset-frontend/src/dashboard/actions/chartCustomizationActions.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #e57967Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
superset-frontend/src/dashboard/actions/chartCustomizationActions.ts
Outdated
Show resolved
Hide resolved
Code Review Agent Run #4ba813Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Can we add some test cases? |
|
🎪 Showtime deployed environment on GHA for ac87ca2 • Environment: http://35.163.128.103:8080 (admin/admin) |
| [...allKeys].forEach(filterKey => { | ||
| // Skip chart customization filters - they are handled separately by saveChartCustomization | ||
| // which triggers queries only for affected charts | ||
| if (filterKey.startsWith('chart_customization_')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty fragile. Any better check? Maybe centralizing it?
SUMMARY
This change prevents request storms caused by Dynamic Group By chart customizations by aligning chart refresh behavior with the existing native filters pattern.
Previously, applying or saving chart customizations triggered a forced re-query of all charts on the dashboard, regardless of whether they were affected. This caused unnecessary request fan-out and excessive /api/v1/chart/data traffic on dashboards with many charts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:

AFTER;

TESTING INSTRUCTIONS
1)Open a dashboard with multiple charts (ideally 10+).
2)Configure Dynamic Group By / Chart Customization for one or more charts.
3)Open the browser DevTools → Network tab (filter by /api/v1/chart/data).
4)Apply the chart customization.
Verify:
1)Only charts affected by the customization issue /api/v1/chart/data requests.
2)Unaffected charts do not re-query.
3)Each affected chart triggers only one request per apply action.
4)No repeated or cascading requests are observed.
ADDITIONAL INFORMATION